[PM-18235] Add PersonalOwnershipPolicyRequirement #5439
Conversation
…nt if the PolicyRequirements flag is enabled Update unit tests
…th feature flag - Add support for checking personal ownership policy using PolicyRequirementQuery when feature flag is enabled - Update CipherService constructor to inject new dependencies - Add tests for personal vault restrictions with and without feature flag
|
New Issues (4)Checkmarx found the following issues in this Pull Request
Fixed Issues (24)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5439 +/- ##
==========================================
+ Coverage 44.52% 44.55% +0.03%
==========================================
Files 1538 1539 +1
Lines 70582 70601 +19
Branches 6316 6318 +2
==========================================
+ Hits 31426 31457 +31
+ Misses 37810 37797 -13
- Partials 1346 1347 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...nsole/OrganizationFeatures/Policies/PolicyRequirements/PersonalOwnershipPolicyRequirement.cs
Outdated
Show resolved
Hide resolved
| @@ -26,7 +29,8 @@ public class ImportCiphersCommand : IImportCiphersCommand | |||
| private readonly ICollectionRepository _collectionRepository; | |||
| private readonly IReferenceEventService _referenceEventService; | |||
| private readonly ICurrentContext _currentContext; | |||
|
|
|||
| private readonly IPolicyRequirementQuery _policyRequirementQuery; | |||
| private readonly IFeatureService _featureService; | |||
|
|
|||
| public ImportCiphersCommand( | |||
There was a problem hiding this comment.
🌱 This constructor now has 10 dependencies. That's a good indication that this code is doing too much and a refactoring is in order.
❓ What would you recommend changing if you could? Dream big!
There was a problem hiding this comment.
Excellent observation! I think an easy thing to do would be to separate the individual vault import and the org vault import each into its own command. From there we could extract the validation logic into a validation class.
.../OrganizationFeatures/Policies/PolicyRequirements/PersonalOwnershipPolicyRequirementTests.cs
Outdated
Show resolved
Hide resolved
test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs
Outdated
Show resolved
Hide resolved
| @@ -139,8 +147,11 @@ public async Task SaveDetailsAsync(CipherDetails cipher, Guid savingUserId, Date | |||
| else | |||
| { | |||
| // Make sure the user can save new ciphers to their personal vault | |||
There was a problem hiding this comment.
Non-blocker: I know it's not part of your scope, but can we replace this comment with a meaningful variable name? I think your variable IsPersonalVaultRestricted is a good option.
| { | ||
| [Theory, AutoData] | ||
| public void DisablePersonalOwnership_IsFalse_IfNoPersonalOwnershipPolicies( | ||
| [PolicyDetails(PolicyType.RequireSso)] PolicyDetails otherPolicy1, |
There was a problem hiding this comment.
Maybe we should follow the should-when naming pattern for consistency?
Consider: DisablePersonalOwnership_ShouldNotBeEnforced_WhenNoPersonalOwnershipPoliciesExist.
There was a problem hiding this comment.
I have renamed them to follow my usual pattern of <MethodName>_<WithSomething>_<ExpectedResult>
| } | ||
|
|
||
| [Theory, AutoData] | ||
| public void DisablePersonalOwnership_Not_EnforcedAgainstProviders( |
There was a problem hiding this comment.
Consider using this name or something similar: DisablePersonalOwnership_ShouldNotBeEnforced_WhenUserIsAProvider.
…rity - Improve test method names to better describe their purpose and behavior - Rename methods to follow a more descriptive naming convention - No functional changes to the test logic
|
@JimmyVo16 thanks for the review! I've addressed your comments, let me know if you want to discuss anything further |
|
Putting this back in draft as it'll require a refactor with the eventual changes from #5445 |
…rement # Conflicts: # src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyServiceCollectionExtensions.cs # src/Core/Tools/ImportFeatures/ImportCiphersCommand.cs # test/Core.Test/Vault/Services/CipherServiceTests.cs
- Add PersonalOwnershipPolicyRequirementFactory to replace static Create method - Simplify policy requirement creation logic - Update PolicyServiceCollectionExtensions to register new factory - Update ImportCiphersCommand to use correct user ID parameter - Remove redundant PersonalOwnershipPolicyRequirementTests
|
|
This is again ready for review. I've implemented the same changes as in #5445 |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18235
📔 Objective
Implement
PersonalOwnershipPolicyRequirementand use it on code checking if the "Remove individual vault" applies to the user.This standardizes policy enforcement by following our established
PolicyRequirementpattern, ensuring consistent handling of role/status exemptions.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes